-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Use PPrint to handle printing of REPL output values #23849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
CC @odersky @hamzaremmal as we discussed this when I visited lausanne |
@lihaoyi we already require Java >= 17, the |
project/Build.scala
Outdated
val downloads = Seq( | ||
"https://repo1.maven.org/maven2/com/lihaoyi/pprint_3/0.9.3/pprint_3-0.9.3-sources.jar", | ||
"https://repo1.maven.org/maven2/com/lihaoyi/fansi_3/0.5.1/fansi_3-0.5.1-sources.jar", | ||
"https://repo1.maven.org/maven2/com/lihaoyi/sourcecode_3/0.4.3-M5/sourcecode_3-0.4.3-M5-sources.jar", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...wait, is 0.4.3-M5 a stable version? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to bring it to stable before we depend on it in the compiler repo 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's almost a year old, so I guess so haha. I can tag a stable version if you would like, but the contents of the sourcejar will be unchanged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's become stabilised, by all means. 👍
I'd rather avoid milestone versions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @Gedochao. A stable version should be used here. Also, @lihaoyi what are the versioning scheme these 3 libraries follow? I'm not a fan of cloning the sources and change the package name. I prefer to just have a dependency and use the actual library (which we do for jline and will soon do for asm too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, others have raised concerns in the past about using Scala libraries in the compiler codebase affecting the bootstrapping process. By building from source, we treat it effectively as Dotty's own source files, removing any divergence in the code paths: they are treated identically to scala3's own sources. If scala3 can compile itself, it should be able to compile these sources without issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why has it been downloaded every time?
- Seems no checkmd5?
- Extract the common version to fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should indeed compile these from sources. We can depend on binaries for Java libraries (hence jline and asm are fine), but not for Scala libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should indeed compile these from sources. We can depend on binaries for Java libraries (hence jline and asm are fine), but not for Scala libraries.
Could you explain why? I thought Scala is maintaining backwards binary/tasty compatibility. Doesn't that mean we shohld always be able to depend on older scala 3 jars in the scala3 compiler regardless of how kuch bootstrapping we do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because circular dependencies are evil. They're not too evil across time (Av2 -> Bv1 -> Av1), but they're still difficult to reason about.
And even though Scala 3 will forever be backward compat, an eventual Scala 4 wouldn't. We shouldn't paint our build into a corner. Scala 2 tried this several times over its lifetime, and rolled back every time. It's a massive pain every time it happens. There would need to be a huge upside to depending on a binary for that to be offset.
@Gedochao the |
@lihaoyi I'm bumping the version required by the CB to Java 17. |
Note: I like the visible results very much, looking forward to how this PR evolves. |
var in: InputStream = null | ||
var zis: ZipInputStream = null | ||
try { | ||
in = new BufferedInputStream(conn.getInputStream) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the using
resource API here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If @hamzaremmal is bumping the required version to Java 17, I'm hoping to delete al of this and it'll collapse down to a single line
I think that using the existing sources with patches is fine for POC, but before merging that I'd rather prefer embedding the sources into this project if possible (including all required acknowledgments and license/author notices). The current code from Also by storing sources in this repo it would be easier to compare changes in case of upcoming upgrades. Same approach is done in Scala Native for dealing with core dependencies like libunwind (see it's download&patch script ) It would also make it easier to introduce bug fixes or adjustments if needed without forcing a new release of pprint of its dependencies. |
@WojciechMazur the tradeoff between vendoring the source v.s. downloading it on the fly and patching it is as follows:
There's no strictly better answer, but I think the tradeoffs of downloading and patching are more useful: it's not that hard to look inside |
Given you are aiming for source dependency instead of binary, consider the possible copyright sideffects. |
I think the current is fine, or we can use git submodule? |
@@ -5,7 +5,7 @@ scala> NInt(23) | |||
val res0: NInt = NInt@17 | |||
|
|||
scala> res0.toString | |||
val res1: String = NInt@17 | |||
val res1: String = "rs$line$1$NInt@17" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what's happening here before this PR, but there must be some super sketchy stdout-regexing happening to mangle the .toString
so it looks different when return
ed or println
-ed.
scala> res0.toString
val res1: String = NInt@17
scala> println(res0.toString)
rs$line$1$NInt@17
The new behavior is probably better: we special case return
ing because it uses pprint
, println
is just println
, and if someone wants pprint
themselves they can use dotty.shaded.pprint.log
|
||
scala>:settings -Vrepl-max-print-characters:10 | ||
|
||
scala> 1.to(10).mkString | ||
val res1: String = 123456789 ... large output truncated, print value to show all | ||
val res1: String = "12345678910" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old -Vrepl-max-print-elements
and -Vrepl-max-print-characters:10
don't work with PPrint
. Instead, we can control the max width
and height
before truncation. As a first pass I'd say we can do that in a follow up, but if people really want I can add those -Vrepl-width
and -Vrepl-height
in this PR
The current failure doesn't seem to have any error message that I can find. Can anyone help me take a look and see what's wrong? https://github.com/scala/scala3/actions/runs/17377385477/job/49326734244?pr=23849 |
Since the project REPL is now primarily for project use, I'd suggest keeping the REPL output simple, and add a Then the currently absent There are already issues with reproduction for tickets involving the usual REPL snippet wrapping and importing from history, let alone |
The test says it's testing
is that due to
at 8750 |
If someone wants the raw toString of a value, it's only a single Furthermore, as you can see the default REPL is nowhere near simple: it does exactly the same thing that PPrint does! It recursively traverses Seqs, truncates Lists, quotes strings, sanitizes ansi codes, manually prints Arrays, etc. The underlying approach is basically identical: a runtime recursive traversal of PPrint just does the same thing better, with its 10 year old implementation being significantly prettier than
And for non-common data types it falls back to the |
This PR demonstrates using the https://github.com/com-lihaoyi/PPrint library to handle pretty-printing of values in the REPL.
Visible improvements:
Seq("")
andSeq()
is no longer identical (sometimes!)'X'
are properly pretty-printed with quotespprint
's own internal highlighter:StringColor
andLiteralColor
as green rather than red. This should help avoid the red of literals being visually confused with the red of error messages when pretty printing code during compilation errors, which is something I have had problems with in the pastFoo
orSeq
orList
, since the vast majority of these identifiers are likely to be the companion object of types, and highlighting them helps greatly in visually finding your way around pretty-printed data structuresBefore:
After:
Notes:
sourceGenerators
. This requires a bit of patching to work around-Xexplicit-nulls
and-Xfatal-warnings
, but otherwise is straightforward and means for all intents and purposes it's just part of the Dotty codebase. We mangle the package paths to make themdotty.shaded.*
packages to avoid conflict with user codePPrint
can be configured, e.g. we can decide whether we want to print field names or not. By default it prints field names for anycase class
with more than1
fieldI turned off
-Xfatal-warnings
so I can build this on my laptop with Java 21, but we can revert that before merging. This PR would definitely need some cleanup before merging, but in principle it seems to workTODO/Future-Work:
show(...)
) to bypass the max height. For now, it's fixed at the default width of 100 columnsos-lib
and other libraries withinscala3-compiler
by building them from sourcefansi
elsewhere in the dotty codebase. e.g. the highlighting of stack traces via the code syntax highlighter is super ugly and could be cleaned up: